Hi @romainruaud. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

All committers have signed the CLA.

@romainruaud @maghamed current implementation approach does not look as a robust solution to me.

Most probably, it will need a huge refactoring on next stable (2.4) :

It would be pretty hard if not impossible. Much better solution is to implement Elasticsearch 6 support as a new module, possibly disabled by default, old module may be deprecated as a whole when new one is ready.

@orlangur I agree with you on some points.

It's the whole Elasticsearch "abstraction" and the Search part of Magento framework (if I understood properly, ES will be the only supported engine now that MySQL fulltext search implementation is facing deprecation too ?) that has to be revamped in my opinion :

• Search Requests definitions (generic, to be able to query anything)
• Search Indices definition (generic, to be able to index anything)
• Search Query building with abstraction of all ES query and aggregation DSL types
• Properly customizable search relevance
• and so on

We can (and we'd enjoy to) help on these topics, since we already built everything listed above in our Open Source implementation of Elasticsearch (which is widely known and used by the community, even documented in the Magento2 devdocs) : https://github.com/Smile-SA/elasticsuite

But this is a huge amount of work to be done and to coordinate.

And for now, Elasticsearch 5 is EOL on 03/11 : https://www.elastic.co/support/eol

So the goal of this PR was to provide support of ES 6 to the community as soon as possible.

Let me know how you wanna process on this PR.

In any cases, I'm not sure there should be a new module "dedicated to Elasticsearch 6", or we will face the same mess on a near future when we will begin to write another module "dedicated to Elasticsearch 7" which is inheriting/extending the other one.

Imho there should be only one module that always handle the latest stable version of Elasticsearch, and that remains compatible with the previous "still maintained" versions with a Deprecation mechanism based on plugins. (So for now, module should fit with ES 6, and handle ES 5 specificity with plugins).
Once a new version of ES is out, we flush all the deprecation plugins, update the default implementation, and build new deprecation plugins. (when ES 7 is out, we provide default implementation of Elasticsearch 7, implement ES 6 specifics as deprecation plugins, and get rid of 5.x completely, because it's EOL). And on and on.

Maybe this part should even be inside Framework directory, or managed on a separate lifecycle.

Product (and more) indexing & querying should be in a separate (most agnostic as possible) modules also.

Let me know,

Best regards

@romainruaud https://github.com/magento/magento2/tree/2.3-develop/app/code/Magento/Tinymce3 is a good example of approach I described.

With all-in-one module it's pretty hard to determine which code parts are responsible for which Elasticsearch version and which versions are actually supported in each particular moment of time.

Elasticsearch6 may be a new standalone module containing everything needed for v6 support. Depending on strategy chosen It may contain plugins for older versions support or such code should be placed in Elasticsearch5 module only.

The same way, when Elasticsearch7 module will be introduced it should provide full support when enabled by itself and may be based on some older code (but not inherit or extend it).

It's enough if only one of Elasticsearch5 / Elasticsearch6 / Elasticsearch7 may be enabled at a time.

But with such an approach you will end up with several modules actually sharing 95+% of their code, just like a big copy-paste, and will end up having to fix any issue on all of these nearly identical modules.

Why would you treat supported Elasticsearch versions differently than PHP versions (or any other component) ?

Supporting the latest and "old-stable" on a single module (with proper deprecation mechanism) is enough for me, and seems far easier to maintain.

But we are now a bit far from the initial topic.

Discussed with @maghamed :

• I'll rework this PR and provide it as a separated module (which will contain only ES6 related logic).

• Later on, we (or more exactly Magento people) will have to decide which support policy to have with ES versions (either support only latest & old-stable, like for PHP or support more versions with as many separated modules as needed).

I let Igor correct me if needed.

Regards

@romainruaud I agree with you that it should be only one module that supports what is still being supported.
I understand that having several modules allows end users to use older versions of ES, but that opens those to security issues in the future. And if end users are not updating ES, most likely they are not updating Magento either.

@CajuCLC that's not always true since software versions are maintained and upgraded by hosting companies and Magento updates are up to the end users.

@miguelbalparda That goes back to what's the right way to maintain your app. IMHO the best way is to maintain everything update so there is minimal security risk.
Does Magento want to make everyone happy? Then why not support PHP 5.3 for all versions then?
You can't make everyone happy, but you can make sure your system is up to date in terms of security. Most of hosting companies will also support older versions of software for some time even if it's already EOL just so it gives their customers time to update.
It all comes down to security. Does Magento plan to leave support to software versions that can be exploited in the future or force customers to update to new versions to get best in class security?

OK @maghamed I think I updated my PR before you drop your comment on the issue :)

It's now built as a separate module. Nothing altered in the old Magento\Elasticsearch module and everything specifically needed for ES6 in a new one.

Just one point : I'm not completely aware about what to do to ensure the new module will be properly created on the Magento2 composer private repo.

Feel free to ask anything you want more.

Regards

@romainruaud Hello. I've created simple product with sku = 'Simple product 1' http://prntscr.com/mqukzb
but in advanced search by sku product isn't founded http://prntscr.com/mqultq .
Is it expected behavior ?

@IvanPletnyov this use case does not seem to work on other Elasticsearch implementations (2.x or 5.x).

So most probably, it has never worked as expected.

If it's easy to fix I can have a look but unfortunately, the main topic of this PR is not to fix all existing functional bugs on Elasticsearch implementation, but rather to ensure compatibility with current stable version of ES 6.

Regards

@romainruaud Okay, thanks for the answer.

Hi @maghamed, thank you for the review.
ENGCOM-4389 has been created to process this Pull Request

@romainruaud thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

Hi @romainruaud, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

So the ES6 will be supported with version 2.3.2? Thanks romainruaud for the great work! 👍

@darksummenors I think Magento is working internally to make it reach the 2.3.1 release pipeline but it's out of my scope. Maybe I was too much "just-in-time" for 2.3.1 if they flagged it for the next release.

Probably @maghamed can clarify this point.

Regards

@romainruaud - thanks for working on this.

It has been added to 2.3.1 (noticed it release notes)!

@darksummenors - also 2.2.8